Array.prototype.unshift does not marshal parameters correctly #2201
Conversation
|
LGTM. Thanks. |
|
It looks like Akrosh fixed this in #2134 but did not merge it. He takes a slightly different approach, by avoiding the if (JavascriptArray()) path if we have a cross site object. I am fine with either fix, do you have any preference? |
|
I like this PR because it includes a test (and Akrosh is on vacation until the new year) -- but of course, you could adopt his change if you decided that was the better approach. |
|
@tcare Should this be retargeted to release/1.4? |
|
Yes, you're right. I will await Paul's additional offline signoff and push there. |
|
After speaking with Curtis, Akrosh's fix in #2134 is correct. I will check that in. |
|
Maybe update this PR to defer to that fix but still add the tests? |
|
Yep that's what 348cd33 is :) |
|
@tcare oh gotcha, didn't notice the update |
| return res; | ||
| } | ||
| if (JavascriptArray::Is(args[0])) | ||
| if (JavascriptArray::Is(args[0]) && !JavascriptArray::FromVar(args[0])->IsCrossSiteObject()) |
There was a problem hiding this comment.
Looks like this is still present in this change?
There was a problem hiding this comment.
That's Akrosh's fix. Mine was in FillFromArgs.
There was a problem hiding this comment.
Ok I guess it will just merge down. Anyway you'll want to retarget this PR to release/1.4 and rebase.
There was a problem hiding this comment.
Ah I missed this targeted master. Will update
|
You can now resolve the CI issues as described here: #2332 (comment) |
Fixes OS 9357224. Array.prototype.unshift does not take the slow path if 'this' is a cross-site object.
…ers correctly Merge pull request #2201 from tcare:unshift Fixes OS 9357224. Array.prototype.unshift prepends array values to 'this' array by calling JavascriptArray::FillFromArgs. FillFromArgs makes the assumption that values are already marshalled to the same context as 'this'. It is possible to have a situation in Array.prototype.unshift where the source array is in another context than the parameters passed in, triggering the assert. Other users of FillFromArgs (e.g. new Array()) don't seem to be able to hit this situation. Fix is to marshal values as we iterate in FillFromArgs.
…rshal parameters correctly Merge pull request #2201 from tcare:unshift Fixes OS 9357224. Array.prototype.unshift prepends array values to 'this' array by calling JavascriptArray::FillFromArgs. FillFromArgs makes the assumption that values are already marshalled to the same context as 'this'. It is possible to have a situation in Array.prototype.unshift where the source array is in another context than the parameters passed in, triggering the assert. Other users of FillFromArgs (e.g. new Array()) don't seem to be able to hit this situation. Fix is to marshal values as we iterate in FillFromArgs.
|
@tcare thanks for taking care of this. |
6 participants
Fixes OS 9357224.
Array.prototype.unshift prepends array values to 'this' array by calling JavascriptArray::FillFromArgs. FillFromArgs makes the assumption that values are already marshalled to the same context as 'this'. It is possible to have a situation in Array.prototype.unshift where the source array is in another context than the parameters passed in, triggering the assert. Other users of FillFromArgs (e.g. new Array()) don't seem to be able to hit this situation. Fix is to marshal values as we iterate in FillFromArgs.